Add `error-prone.picnic.tech` #5006

Pankraz76 · 2025-09-29T11:07:29Z

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes

Links:

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

documentation/src/main/java/example/domain/Person.java

Pankraz76

2ct.

Pankraz76 · 2025-10-04T15:04:48Z

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

-				// The findings of this check are subjective because a named constant can be more readable in many cases
-				"UnnecessaryLambda",
-
-				// Resolving findings for these checks requires ErrorProne's annotations which we don't want to use


Assumed this is for the whole block as the block is some kind of pattern here but its a nice trap, assuming the comment only regards to AnnotateFormatMethod.

Pankraz76 · 2025-10-04T15:05:07Z

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

-				"MissingSummary",
+				"BadImport", // This check is opinionated wrt. which method names it considers unsuitable for import which includes a few of our own methods in `ReflectionUtils` etc.
+				"UnnecessaryLambda", // The findings of this check are subjective because a named constant can be more readable in many cases.
+				"AnnotateFormatMethod", // We don't want to use ErrorProne's annotations.


inline this is an clear intent and not an obsolete question.

Pankraz76 · 2025-10-04T15:06:49Z

...r-engine/src/main/java/org/junit/jupiter/engine/extension/PreInterruptThreadDumpPrinter.java

 					sb.append(NL);
 					// Use the same prefix as java.lang.Throwable.printStackTrace(PrintStreamOrWriter)
 					sb.append("\tat ");
-					sb.append(stackTraceElement.toString());


assuming RedundantStringEscape is the fix.

Did you change this class manually or by applying a patch?

This patch is error-prone — it depends on the library’s behavior instead of individual, opinionated interpretation.
It would probably still pass the build even if reverted, which is exactly what we want to avoid. The setup isn’t really complete yet, and we’re still aiming for the proper rewrite equivalent.

works as expected, failing the build on corruption:

meaning on cloud, when if local fixes are active, but not pushed, it will fail in the quality gate.

still this is kind of wack having to know the trick about the custom env, but okey.

Rewrite directly outputs the command how to apply the suggested/enforced change upfront.

11:18:41: Executing 'assemble clean'… Starting Gradle Daemon... Gradle Daemon started in 1 s 213 ms > Task :junit-jupiter-api:javadoc FROM-CACHE > Task :junit-jupiter-engine:compileJava /Users/vincent.potucek/IdeaProjects/junit-framework/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/PreInterruptThreadDumpPrinter.java:52: warning: [RedundantStringConversion] Avoid redundant string conversions when possible sb.append(stackTraceElement.toString()); ^ (see https://error-prone.picnic.tech/bugpatterns/RedundantStringConversion) Did you mean 'sb.append(stackTraceElement);'? > Task :junit-platform-console:compileJava > Task :junit-platform-console:classes > Task :junit-jupiter-api:javadocJar > Task :junit-jupiter-api:assemble > Task :junit-jupiter-engine:compileJava FAILED error: warnings found and -Werror specified 1 error 1 warning > Task :junit-platform-console:shadowJar [Incubating] Problems report is available at: file:///Users/vincent.potucek/IdeaProjects/junit-framework/build/reports/problems/problems-report.html Deprecated Gradle features were used in this build, making it incompatible with Gradle 10. You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins. For more on this, please refer to https://docs.gradle.org/9.1.0/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation. 135 actionable tasks: 77 executed, 11 from cache, 47 up-to-date FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':junit-jupiter-engine:compileJava'. > Compilation failed; see the compiler output below. /Users/vincent.potucek/IdeaProjects/junit-framework/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/PreInterruptThreadDumpPrinter.java:52: warning: [RedundantStringConversion] Avoid redundant string conversions when possible sb.append(stackTraceElement.toString()); ^ (see https://error-prone.picnic.tech/bugpatterns/RedundantStringConversion) Did you mean 'sb.append(stackTraceElement);'? 1 error 1 warning BUILD FAILED in 59s 11:19:40: Execution finished 'assemble clean'.

marcphilipp · 2025-10-04T18:33:25Z

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

+		errorproneArgs.add("-XepOpt:Refaster:NamePattern=^(?!.*Rules\\$).*") // might consider.
+		if (getenv("IN_PLACE").toBoolean()) {
+			errorproneArgs.addAll(
+				"-XepPatchLocation:IN_PLACE", // why only certain picnic rules apply?


How does it know where to find the source files to patch? Does patching actually work? This comment suggests otherwise: tbroyer/gradle-errorprone-plugin#38 (comment)

It compiles, which means it’s quite involved and includes all the necessary information, producing output without any errors. Some of the changes might be critical, so it’s recommending to review the automatically generated modifications — just as rewrite advises — since these are machine suggestions and not guaranteed to be perfect. Double-checking is always necessary.

That’s why this feature is opt-in. It’s designed to catch issues, highlight them, and allow to fix the build with a single simple command. Just like check and spot have evolved to.

https://errorprone.info/docs/patching

[openrewrite] RecipeNullabilityBestPractices diffplug/spotless#2663

[openrewrite] StringRulesRecipes diffplug/spotless#2656

marcphilipp · 2025-10-04T18:38:35Z

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

 	options.errorprone {
+		allErrorsAsWarnings = true
+		disableWarningsInGeneratedCode = true
+		errorproneArgs.add("-XepOpt:Refaster:NamePattern=^(?!.*Rules\\$).*") // might consider.


What does this do?

Ignore all Refaster Rules for now, as we fail many of them and maintaining a whitelist is too costly.

This means we’ll ignore all rules by default and selectively activate specific ones when needed. Since Refaster rules are not applied automatically by the autofix process, it’s not practical to enforce them at scale.

While rewrite can apply them as shown, error-prone should ideally be able to do the same — so this might just be a configuration or bug issue.

We could fix issues locally using rewrite and then activate Picnic, though that’s quite a hack. You mentioned it’s not allowed to be part of the main pipeline, but we could still integrate it without enabling it. This way, I get my rewrite integration, and you still maintain the rejection of it since it’s not actively applied in the change.

In a sense, this creates a kind of technical "detour," but one could argue that rewrite acts as a refactoring tool that goes beyond even what error-prone can do. At the end, we would benefit from both: rewrite can cover Refaster rules from Picnic (but not all bug patterns), while error-prone focuses on bug patterns. Each tool has its own benefits. One could also see rewrite as a "big brother" running in the cloud, and error-prone as the fast, local feedback tool.

This approach could help address JavaUtilAPIs changes and Java migrations that error-prone doesn’t cover, while still improving the overall codebase and developer experience.

[openrewrite] migrate.util.JavaUtilAPIs diffplug/spotless#2660

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

marcphilipp · 2025-10-04T18:39:59Z

...r-engine/src/main/java/org/junit/jupiter/engine/extension/PreInterruptThreadDumpPrinter.java

 					sb.append(NL);
 					// Use the same prefix as java.lang.Throwable.printStackTrace(PrintStreamOrWriter)
 					sb.append("\tat ");
-					sb.append(stackTraceElement.toString());


Did you change this class manually or by applying a patch?

Pankraz76

thanks for the effort spend.

Pankraz76 · 2025-10-05T08:44:19Z

...r-engine/src/main/java/org/junit/jupiter/engine/extension/PreInterruptThreadDumpPrinter.java

 					sb.append(NL);
 					// Use the same prefix as java.lang.Throwable.printStackTrace(PrintStreamOrWriter)
 					sb.append("\tat ");
-					sb.append(stackTraceElement.toString());


This patch is error-prone — it depends on the library’s behavior instead of individual, opinionated interpretation.
It would probably still pass the build even if reverted, which is exactly what we want to avoid. The setup isn’t really complete yet, and we’re still aiming for the proper rewrite equivalent.

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

Pankraz76 · 2025-10-05T09:05:11Z

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

+		errorproneArgs.add("-XepOpt:Refaster:NamePattern=^(?!.*Rules\\$).*") // might consider.
+		if (getenv("IN_PLACE").toBoolean()) {
+			errorproneArgs.addAll(
+				"-XepPatchLocation:IN_PLACE", // why only certain picnic rules apply?


It compiles, which means it’s quite involved and includes all the necessary information, producing output without any errors. Some of the changes might be critical, so it’s recommending to review the automatically generated modifications — just as rewrite advises — since these are machine suggestions and not guaranteed to be perfect. Double-checking is always necessary.

That’s why this feature is opt-in. It’s designed to catch issues, highlight them, and allow to fix the build with a single simple command. Just like check and spot have evolved to.

https://errorprone.info/docs/patching

[openrewrite] RecipeNullabilityBestPractices diffplug/spotless#2663

[openrewrite] StringRulesRecipes diffplug/spotless#2656

Pankraz76

ready to go.

Pankraz76 · 2025-10-05T10:33:03Z

...form-console/src/main/java/org/junit/platform/console/options/TestDiscoveryOptionsMixin.java


-		@Option(names = {
-				"--exclude-package" }, paramLabel = "PKG", arity = "1", description = "Provide a package to be excluded from the test run. This option can be repeated.")
+		@Option(names = "--exclude-package", paramLabel = "PKG", arity = "1", description = "Provide a package to be excluded from the test run. This option can be repeated.")


Refactoring changes were successfully applied to file:///Users/vincent.potucek/IdeaProjects/junit-framework/junit-platform-console/src/main/java/org/junit/platform/console/options/TestDiscoveryOptionsMixin.java, please check the refactored code and recompile.

> Task :junit-platform-console-standalone:classes UP-TO-DATE /Users/vincent.potucek/IdeaProjects/junit-framework/junit-platform-console/src/main/java/org/junit/platform/console/options/TestDiscoveryOptionsMixin.java:158: Note: [CanonicalAnnotationSyntax] Omit redundant syntax from annotation declarations @Option(names = { ^ (see https://error-prone.picnic.tech/bugpatterns/CanonicalAnnotationSyntax) Did you mean '@Option(names = "--include-package", paramLabel = "PKG", arity = "1", description = "Provide a package to be included in the test run. This option can be repeated.")'? /Users/vincent.potucek/IdeaProjects/junit-framework/junit-platform-console/src/main/java/org/junit/platform/console/options/TestDiscoveryOptionsMixin.java:162: Note: [CanonicalAnnotationSyntax] Omit redundant syntax from annotation declarations @Option(names = { ^ (see https://error-prone.picnic.tech/bugpatterns/CanonicalAnnotationSyntax) Did you mean '@Option(names = "--exclude-package", paramLabel = "PKG", arity = "1", description = "Provide a package to be excluded from the test run. This option can be repeated.")'? /Users/vincent.potucek/IdeaProjects/junit-framework/junit-platform-console/src/main/java/org/junit/platform/console/options/TestDiscoveryOptionsMixin.java:166: Note: [CanonicalAnnotationSyntax] Omit redundant syntax from annotation declarations @Option(names = { ^ (see https://error-prone.picnic.tech/bugpatterns/CanonicalAnnotationSyntax) Did you mean '@Option(names = "--include-methodname", paramLabel = "PATTERN", arity = "1", description = "Provide a regular expression to include only methods whose fully qualified names without parameters match. "'? /Users/vincent.potucek/IdeaProjects/junit-framework/junit-platform-console/src/main/java/org/junit/platform/console/options/TestDiscoveryOptionsMixin.java:171: Note: [CanonicalAnnotationSyntax] Omit redundant syntax from annotation declarations @Option(names = { ^ (see https://error-prone.picnic.tech/bugpatterns/CanonicalAnnotationSyntax) Did you mean '@Option(names = "--exclude-methodname", paramLabel = "PATTERN", arity = "1", description = "Provide a regular expression to exclude those methods whose fully qualified names without parameters match. "'? /Users/vincent.potucek/IdeaProjects/junit-framework/junit-platform-console/src/main/java/org/junit/platform/console/options/TestDiscoveryOptionsMixin.java:220: Note: [CanonicalAnnotationSyntax] Omit redundant syntax from annotation declarations @Option(names = { ^ (see https://error-prone.picnic.tech/bugpatterns/CanonicalAnnotationSyntax) Did you mean '@Option(names = "--config-resource", paramLabel = "PATH", arity = "1", description = "Set configuration parameters for test discovery and execution via a classpath resource. This option can be repeated.")'? Refactoring changes were successfully applied to file:///Users/vincent.potucek/IdeaProjects/junit-framework/junit-platform-console/src/main/java/org/junit/platform/console/options/TestDiscoveryOptionsMixin.java, please check the refactored code and recompile. > Task :junit-platform-console:shadowJar > Task :junit-platform-console:jar SKIPPED > Task :junit-platform-console-standalone:extractThirdPartyLicenses > Task :junit-platform-console:javadoc FROM-CACHE > Task :junit-platform-console:javadocJar > Task :junit-platform-console:assemble > Task :junit-platform-console-standalone:shadowJar > Task :junit-platform-console-standalone:jar SKIPPED > Task :junit-platform-console-standalone:javadoc NO-SOURCE > Task :junit-platform-console-standalone:javadocJar > Task :junit-platform-console-standalone:assemble [Incubating] Problems report is available at: file:///Users/vincent.potucek/IdeaProjects/junit-framework/build/reports/problems/problems-report.html Deprecated Gradle features were used in this build, making it incompatible with Gradle 10. You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins. For more on this, please refer to https://docs.gradle.org/9.1.0/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation. BUILD SUCCESSFUL in 7s 168 actionable tasks: 81 executed, 33 from cache, 54 up-to-date Consider enabling configuration cache to speed up this build: https://docs.gradle.org/9.1.0/userguide/configuration_cache_enabling.html 12:32:08: Execution finished 'clean assemble'.

Pankraz76 · 2025-10-05T10:35:35Z

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

+				"-XepPatchLocation:IN_PLACE", // why only certain picnic rules apply?
+				"-XepPatchChecks:" +
+						"AddNullMarkedToPackageInfo," +
+						"AlwaysThrows," +


For sure, we don’t need all of these, but technically or ideally, some definitely apply.

Continuing to do so is the natural outcome of the plugin being consistent—failing the build if something goes wrong—thus automating the process and reinforcing automation.

Pankraz76 · 2025-10-05T10:36:50Z

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

+				"OptionalOrElseGet",
+				"PrimitiveComparison",
+				"StaticImport",
+				"TimeZoneUsage",


These are failing and cannot be fixed through patching, which is somewhat of a mystery.

Pankraz76 commented Sep 29, 2025

View reviewed changes

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts Outdated Show resolved Hide resolved

This was referenced Sep 29, 2025

Add rewrite support for errorprone.refasterrules #5002

Open

Fix DirectReturn gradle/gradle#35201

Closed

Pankraz76 force-pushed the extend-errorprone branch from 5577787 to 84526f8 Compare October 3, 2025 21:44

Pankraz76 changed the title ~~add refaster-runner~~ Add error-prone.picnic.tech Oct 3, 2025

Pankraz76 force-pushed the extend-errorprone branch from 84526f8 to bad204a Compare October 3, 2025 21:45

Pankraz76 marked this pull request as ready for review October 3, 2025 21:45

Pankraz76 mentioned this pull request Oct 3, 2025

Add rewrite support for errorprone.refasterrules gradle/gradle#35203

Closed

Pankraz76 force-pushed the extend-errorprone branch from bad204a to 7edc8cb Compare October 3, 2025 21:57

Pankraz76 changed the title ~~Add error-prone.picnic.tech~~ Add error-prone.picnic.tech covering S1488: Local variables should not be declared and then immediately returned or thrown Oct 3, 2025

marcphilipp requested changes Oct 4, 2025

View reviewed changes

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts Outdated Show resolved Hide resolved

documentation/src/main/java/example/domain/Person.java Outdated Show resolved Hide resolved

Pankraz76 force-pushed the extend-errorprone branch 2 times, most recently from cb987d5 to c36f592 Compare October 4, 2025 15:02

Pankraz76 commented Oct 4, 2025

View reviewed changes

Pankraz76 requested a review from marcphilipp October 4, 2025 15:08

Pankraz76 force-pushed the extend-errorprone branch 2 times, most recently from 8132ffc to 3337a15 Compare October 4, 2025 16:14

Pankraz76 changed the title ~~Add error-prone.picnic.tech covering S1488: Local variables should not be declared and then immediately returned or thrown~~ Add error-prone.picnic.tech Oct 4, 2025

marcphilipp reviewed Oct 4, 2025

View reviewed changes

Pankraz76 commented Oct 5, 2025

View reviewed changes

Add error-prone.picnic.tech

6a78616

Pankraz76 force-pushed the extend-errorprone branch from 3337a15 to 6a78616 Compare October 5, 2025 10:32

Pankraz76 commented Oct 5, 2025

View reviewed changes

Pankraz76 requested a review from marcphilipp October 5, 2025 10:38

This was referenced Oct 6, 2025

Add error-prone.picnic.tech Pankraz76/spotless#3

Merged

Add error-prone.picnic.tech featuring StaticImport diffplug/spotless#2664

Draft

Uh oh!

Add error-prone.picnic.tech #5006

Are you sure you want to change the base?

Add error-prone.picnic.tech #5006

Conversation

Pankraz76 commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Definition of Done

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Pankraz76 Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add `error-prone.picnic.tech` #5006

Add `error-prone.picnic.tech` #5006

Pankraz76 commented Sep 29, 2025 •

edited

Loading

Pankraz76 Oct 4, 2025 •

edited

Loading

Pankraz76 Oct 5, 2025 •

edited

Loading

Pankraz76 Oct 5, 2025 •

edited

Loading